feat(config): implement global config to wire up config command - #1789
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #1789 +/- ##
==========================================
Coverage 94.84% 94.85%
==========================================
Files 143 149 +6
Lines 7120 7360 +240
==========================================
+ Hits 6753 6981 +228
- Misses 367 379 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
db54dba to
b39f26a
Compare
|
typecheck fixed in #1756 (comment) |
b39f26a to
4b819e1
Compare
d60cf0d to
7fdb445
Compare
|
rebased onto create command work. |
AlexanderRichey
left a comment
There was a problem hiding this comment.
I feel that the level of complexity this PR introduces is out of proportion with the functionality gained. Let's sync on this and see if we can develop a simpler approach.
|
I have some prior art here that might be useful to look at: https://github.com/AlexanderRichey/yagss/blob/main/internal/builder/config.go |
|
converted to draft while I rework based on offline discussion from above, will take out of draft when ready for another review. |
c9c3f17 to
c0c3d7a
Compare
e393ae5 to
548979d
Compare
AlexanderRichey
left a comment
There was a problem hiding this comment.
This is looking really good. The main things blocking for me at the moment are (1) supporting (what I think are unnecessary) bool types, when JSON has a native bool type, and (2) adding a whole bunch of code to support isValidKey() when simpler options seem to be available.
| } | ||
| } | ||
|
|
||
| public async set(newConfig: Record<string, unknown>): Promise<GlobalConfig> { |
There was a problem hiding this comment.
Why not use the GlobalConfig type here? That would give us type-safety and any non-GlobalConfig value passed would fail at compile time.
There was a problem hiding this comment.
answered below, but I'm aligned with typing it here.
| export { type GlobalConfigAccessor, type GlobalConfig, type ReadWriteJson } from "./types"; | ||
| export { globalConfigSchema, isValidKey } from "./schemas"; | ||
| export { DefaultGlobalConfigAccessor } from "./accessor"; | ||
| export { FsReadWriteJson } from "./json"; |
There was a problem hiding this comment.
This should probably be in its own json package. We can handle that in a follow up.
There was a problem hiding this comment.
+1, I figure we pull it out once we have another consumer.
| @@ -0,0 +1,110 @@ | |||
| import z from "zod"; | |||
|
|
|||
| /** Zod boolean schema that coerces string values (`"true"`, `"1"`, `"false"`, `"0"`) to booleans. */ | |||
There was a problem hiding this comment.
Is this the current behavior? If not, I don't think we should support this. JSON has bools.
There was a problem hiding this comment.
discussed in person, this was for coercion, but has a bad side effect of accepting odd looking configs.
| }, z.boolean()); | ||
|
|
||
| /** Creates a Zod object schema that coerces JSON strings into objects before validation. */ | ||
| const coerceObject = <TShape extends z.ZodRawShape>(shape: TShape) => |
There was a problem hiding this comment.
I'm not sure I'm following why we need this. We read a JSON string with JSON.parse, which should parse nested objects just fine, if that's what this is intended to handle.
| endpoint: z.string().optional(), | ||
| audit: coercedBoolean.optional(), | ||
| }).optional(), | ||
| installationId: z.uuid().optional(), |
There was a problem hiding this comment.
What's the argument in favor of using coerceObject and coercedBoolean over standard object and bool?
There was a problem hiding this comment.
removed based on offline conversation
|
|
||
| return paths; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not following what all this code is for.
There was a problem hiding this comment.
answered offline.
| /** Returns the current global config. */ | ||
| get(): Promise<GlobalConfig>; | ||
| /** Validates and persists a new config. Throws on invalid shape. */ | ||
| set(newConfig: Record<string, unknown>): Promise<GlobalConfig>; |
There was a problem hiding this comment.
Same as above comment. This should accept a type.
There was a problem hiding this comment.
I was originally thinking its more "honest" since we already validate it internally to accept unknown. This also avoids the cast at the config handler call site. However, I think I agree that its easier to type this, and cast for exceptions so that we get the static check in as many places as possible.
| @@ -21,8 +22,61 @@ export const createConfigHandler = (io: AppIO) => | |||
There was a problem hiding this comment.
Shouldn't the options be enumerated here? Enumerating the options here would be far simpler than the current isValidKey() code.
There was a problem hiding this comment.
I like that idea!
|
Taking into draft while I address the comments and look to simplify. |
|
one other approach I considered is duplicating the schema into: Then using that instead of walking the zod schema to validate keys and paths. It would remove some complexity at the cost of maintaining two configs. However, I feel like building this at runtime is easier and avoids the drift (but does introduce more complexity). Especially since if we go with the explicit duplicate mapping, we'd probably want a unit test verifying they don't drift which might result in writing the same code anyway. |
7188c20 to
cd99d77
Compare
e190e1b to
b298004
Compare
|
Main changes:
|
| /** | ||
| * Default values for the global config. Includes a unique installationId for each process. | ||
| */ | ||
| export const getDefaultGlobalConfig = once<GlobalConfig>(() => ({ |
There was a problem hiding this comment.
Any reason why we can't have this?
const DEFAULT_GLOBAL_CONFIG = {
telemetry: {
enabled: true,
audit: false,
endpoint: "https://telemetry.agentcore.aws.dev",
},
installationId: crypto.randomUUID(),
} as GlobalConfigThere was a problem hiding this comment.
I originally wasn't confident that loading the module in multiple places could cause different installationIds here. It looks like this isn't the case, and module resolution should handle this for us , but thought it was clearer to make this explicit in the code.
However, it sounds like maybe this isn't adding clarity and instead only adds complexity. I'll swap back to the constant.
0c63bb5 to
1501976
Compare
1501976 to
0531cce
Compare
Problem
The CLI is missing a way to view and control global configuration. There is an existing
configcommand, but its not wired up.Solution
globalConfig/module that defines an accessor for global config. The global config is decoupled from the source of the data via an injected dependency.src/router/schemas.tsxto top level/parsingdirectory so that logic can be re-used.telemetry,telemetry.enabled, etc. All of which are derived automatically from the schema definition.Testing
Ex.
Future Work